-
Notifications
You must be signed in to change notification settings - Fork 312
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Proper controller update rate #1105
Proper controller update rate #1105
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I understand this correctly? With this change, the average update rate over a longer period is the desired one, in contrast to previous version with the nearest integer division factor resulting in a constant error of the update rate.
Although, a warning would be maybe fine if the configured update rate cannot be ensured exactly (having a jitter in the commands)
Yes, you are right @christophfroehlich , with the previous version, if your controller is configured to 40Hz and your controller manager runs at 100Hz, then it prints a warning that it would run it ar 50 Hz, and the controller is updated at 50Hz instead of the desired 40Hz. With this PR, you will ensure the 40 Hz rate, but then your period between the update cycles won't be constant as it is not a perfect divisor
Yes for the previous implementation, it is already printed and now, as we achieve what we have configured, this is removed in this PR. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1105 +/- ##
==========================================
- Coverage 31.83% 31.75% -0.08%
==========================================
Files 94 94
Lines 10483 10486 +3
Branches 7136 7146 +10
==========================================
- Hits 3337 3330 -7
Misses 801 801
- Partials 6345 6355 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
|
I'd still throw a warning, because the update rate won't be constant and exactly as expected. |
Sure, makes sense. I can add something like, The update period of the controller won't be constant for every update cycle, unless the update rate is a perfect divisor. Thanks for the feedback @christophfroehlich. Appreciate it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, just few comments.
Co-authored-by: Dr. Denis <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the following up on this!
This PR adds a logic to have a proper controller update rate without needing to have a perfect divisor with respect to the controller manager's update rate